Skip to content

Andrey/adnan/solidus 4.0#3

Open
ashgaliyev wants to merge 72 commits intoadnan/solidus-4.0from
andrey/adnan/solidus-4.0
Open

Andrey/adnan/solidus 4.0#3
ashgaliyev wants to merge 72 commits intoadnan/solidus-4.0from
andrey/adnan/solidus-4.0

Conversation

@ashgaliyev
Copy link
Copy Markdown

No description provided.

forkata and others added 30 commits July 17, 2023 14:55
This is helpful to have when debugging feature specs.

Co-authored-by: benjamin wil <benjamin@super.gd>
Co-authored-by: benjamin wil <benjamin@super.gd>
We need to evaluate when an order should be reported to TaxJar in
multiple places. Currently this logic all lives in the reporting
subscriber module and is unfortunately duplicated in both handlers. In a
future commit we want to introduce a Sync or Retry action for failed
transaction syncs, which will also require that we perform these checks.
This change attempts to extract some of that logic in a `Reportable`
concern and provide a `with_reportable` API which takes a block to
evaluate when all conditions are met.

In the next commit we'll attempt to further extract the pre-requisites
for replacing a transaction on TaxJar into this and possibly other
checks specific to that flow.

Co-authored-by: benjamin wil <benjamin@super.gd>
These conditions were already implicitly true for the
`report_transaction` event action, so we can safely move them into the
shared module which now provides all the checks that need to be
satisfied by an order.

The tests needed a small change to better reflect the state of the
shipment and the order, when the `shipment_shipped` event is fired (i.e.
after the transition of a shipment to the `shipped` state).

Co-authored-by: Benjamin Willems <benjamin@super.gd>
This is the last step of the refactoring work to extract the logic
around `Reportable` orders. This change combines the conditions for
whether a transaction on TaxJar is replaceable into a single public API
we can expose on the `Reportable` module. This will allow us to
condition logic on this check by simply including this module wherever
we need to re-use this logic.
This will allow us to completely remove any conditions from the
reporting subscriber and creates a clean API around executing any code
that needs to check if an order can be reported/replaced.

Co-authored-by: benjamin wil <benjamin@super.gd>
For some reason our feature spec will receive a shipment record which
isn't yet shipped, and that is likely because in our feature specs we
run the jobs for the reporting subscriber async, unlike in our unit
tests where we just assert that a job has been queued.

Firing the event is done synchronously, so this change will ensure that
the service layer that handles generating Carton objects when inventory units
# are actually shipped and also updates the order and shipment states
has been performed (in `Spree::OrderShipping`), before the event is
consumed by our subscriber.

We tried updating the unit specs to expose this, but we were not successful.

Co-authored-by: benjamin wil <benjamin@super.gd>
We are about to make some changes to this file so let's re format these
lines for readability.

Co-authored-by: benjamin wil <benjamin@super.gd>
Our test helper to stub calls to the event system does not work on
Solidus 3.2 and newer where Omnes is the default bus implementation.
This change updates the stubbing to correctly stub out the new system if
that is the one in use.

Co-authored-by: benjamin wil <benjamin@super.gd>
The method we were using previously, was deprecated in Ruby 2.2 and is
removed in 3.x. We should be using the new API here as we now support
Ruby 3.x on this extension.

Co-authored-by: benjamin wil <benjamin@super.gd>
In order to support the new Omnes event system on Solidus 4.x, we are
duplicating our existing subscriber class and renaming it to
`LegacyReportingSubscriber` and updating the implementation of what will
be the new `ReportingSubscriber` to use the new Omnes API.

One change we realized is necessary is to move both subscriber files
to the `lib` folder so they are no longer automatically loaded by the host app.
This means we need to manually require and "activate" the correct
subscriber based on the Solidus version. For this we are introducing a
`solidus_taxjar_legacy_events.rb` initializer for versions of Solidus
where Omnes has not been introduced yet - prior to 3.2.

Co-authored-by: Cameron Day <cameron@super.gd>
Co-authored-by: benjamin wil <benjamin@super.gd>
Solidus 2.11 has been deprecated for a very long time, so we should drop
this before releasing version 1.0 of this extension.

Co-authored-by: benjamin wil <benjamin@super.gd>
Now that Solidus 4.1 is released and Solidus 4.0 was added to the CI
task [^1] for solidus-older we need to bump the version of Ruby used to
3.0.

[^1] solidusio/circleci-orbs-extensions#87
There is something happening when setting up the order for this refund
spec which is causing it to sometimes not get taxes on CI. That results
in a mismatch in the refund amount causing the test to fail.
Prior to this commit, we ran into issues retrieving the latest order
transaction as many contained the same transaction date. To fix, we add
secondary sorting ordered by `created_at`.

Co-authored-by: Alistair Norman <alistair@super.gd>
Co-authored-by: Benjamin Willems <benjamin@super.gd>
This doesn't belong here!

Co-authored-by: Alistair Norman <alistair@super.gd>
Prior to this commit, all line items would be reported to TaxJar,
including returned line items. To fix this, we check that a line item
has at least one inventory unit with a state other than 'returned'.

Co-authored-by: Alistair Norman <alistair@super.gd>
Prior to this change, each request to TaxJar sent all line items. We
should exclude line items that have been returned or cancelled so the
total of the line items still adds up to the order's payment total.

Co-authored-by: Noah Silvera <noah@super.gd>
Prior to this change, the query for inventory units that are not
cancelled or returned was repeated three times so we move the query into
a private method.

Co-authored-by: Alistair Norman <alistair@super.gd>
We want to show the user a reference to the TaxJar refund transaction
that is created when updating an order in TaxJar.
Prior to this commit, we never created a transaction sync log when
replacing an order on TaxJar. Now that a previous commit returns the
refund transaction and order transaction from the reporting class, we can
create the transaction sync log.
Prior to this commit, any errors raised by this job would remain
un-rescued and most likely in someone's error reporting software.
Instead, it's more helpful to create a transaction sync log and show the
error in the Solidus admin.
We should show this information to the user so the sync records in
Solidus match up with the sync records in TaxJar.
VCR is configured to block network requests during test runs. But if
Selenium Manager requires Chrome binaries for feature tests, it makes
requests we need it toamke. For now, let's just ignore the known Google
hosts.
In a subsequent commit we want to use this outside of the reporting
subscriber tests.
Noah-Silvera and others added 30 commits January 2, 2024 16:44
This updates the changelog using the format suggested by
https://keepachangelog.com and breaks up the upcoming changes into a few
separate categories. This should hopefully help users of the extension
easily identify the changes they need to be aware of when upgrading to a
new release, and also help us identify clearly what changes are
unreleased yet.
In a test environment, we discovered that sometimes these jobs would be
run before an order's new totals had been persisted by the
`Spree::OrderUpdater`. This is an effective workaround, but it is just
a workaround.
This extension is not ready for the new admin, so let's not use the new
admin when generating sandbox apps.

Co-authored-by: Alistair Norman <alistair@super.gd>
Co-authored-by: benjamin wil <benjamin@super.gd>
This causes issues with Zeitwerk on the Solidus `main` branch, because
it lives in the `app/overrides` folder which is loaded by default as per
Rails convention. Switching the file to `.deface` ensures that Zeitwerk
does not try to load it.

Co-authored-by: Alistair Norman <alistair@super.gd>
This fixes the `bin/sandbox` when running with SOLIDUS_BRANCH=`main`.
Previous to these changes, the building of the sandbox app would fail
with an error finding `/config/routes.rb` during the running of the
`solidus:install` task as part of the script. It's not immediately clear
which of these changes fixed the issue, but this is what
`solidus_dev_support` generates now for new extensions, so let's run
with it.

Co-authored-by: Alistair Norman <alistair@super.gd>
We should really switch to the new frontend, but until we can figure out
why generating the sandbox app with the new frontend fails let's add
back the old one. This is also consistent with the dummy app so may help
with debugging.

Co-authored-by: benjamin wil <benjamin@super.gd>
Our checkout and post-checkout feature specs share some `before` setup,
and I wanted to extract it into a shared context to make the
spec-specific set up more clear.
While trying to make this spec pass and investigate an issue with
tax calculations we found during manual testing, I wanted to refactor
this spec so that:

1. It would be more clear which line items we were reimbursing for.
2. It would cover the case where a line item would be removed as well as
   the case where a line item would not be removed but its quantity
   would be changed

This test is pending as the reimbursement amounts populated are
currently wrong ($10.00 instead of $10.89):

    Failures:

      1) Refunding an order adds tax calculated by TaxJar to the order total
         Failure/Error: expect(find(".reimbursement-refund-amount")).to have_content("$10.89")
           expected to find text "$10.89" in "$10.00"
         # ./spec/features/spree/admin/refund_spec.rb:72:in `block (2 levels) in <top (required)>'

I removed the VCR cassette because we will need to re-record it as part
of making the test pass.
Before this commit, we were excluding returned and cancelled inventory
units in two circumstances:

  1. For tax calculations
  2. For TaxJar's transaction reporting dashboard

But this was incorrect. To explain this, we must look at orders with
customer returns on them.

We want to exclude returned and cancelled inventory units for reporting
purposes (2), but we must continue calculating tax for any returned or
cancelled units (1). The reason we must continue calculating tax is so
that we have a record of how much tax must be reimbursed to customers
when we generate new `Spree::Reimbursement`s from the admin.

A bit more context:

Before this change, whenever `order.recalculate` is called, we would
recalculate taxes, excluding returned or cancelled items. This in a
returned line item's `#additional_tax_total` being fully zeroed out. A
partially returned line item's tax total would be in an even
harder-to-understand state.

Co-authored-by: Adam Mueller <adam@super.gd>
Co-authored-by: Noah Silvera <noah@super.gd>
This spec is intermittently failing on CI, and it's incredibly hard to
reproduce why locally.

We need to keep moving so pending this on CI for now

Co-authored-by: benjamin wil <benjamin@super.gd>
This resolves the remaining checklist items in issue SuperGoodSoft#207.

Closes SuperGoodSoft#207.
Now that we're officially in a stable release, let's show it off. It's
also helpful for users of the extension to see the latest published
version.
# Conflicts:
#	app/overrides/spree/admin/shared/_configuration_menu.rb
#	app/subscribers/super_good/solidus_taxjar/spree/reporting_subscriber.rb
#	lib/generators/super_good/solidus_taxjar/install/install_generator.rb
#	lib/super_good/engine.rb
…er-submissions

Fix TaxJar order submissions
…culation

Add discount to amount calculation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants